[MS][clang] Make sure vector deleting dtor calls correct operator delete #133950

Fznamznon · 2025-04-01T17:58:59Z

During additional testing I spotted that vector deleting dtor calls operator delete, not operator delete[] when performing array deletion. This patch fixes that.

github-actions · 2025-04-01T18:01:30Z

✅ With the latest revision this PR passed the C/C++ code formatter.

rnk

Thanks, looks good to me!

DKLoehr · 2025-04-08T16:56:03Z

We're seeing several errors building chromium that bisect to this change: error: member 'operator delete[]' found in multiple base classes of different types. An example build is here; the output is

../../third_party/pdfium/xfa/fxfa/cxfa_ffexclgroup.cpp:16:19: error: member 'operator delete[]' found in multiple base classes of different types
   16 | CXFA_FFExclGroup::~CXFA_FFExclGroup() = default;
      |                   ^
../../third_party/pdfium/xfa/fxfa/cxfa_ffexclgroup.cpp:16:41: note: in defaulted destructor for 'CXFA_FFExclGroup' first required here
   16 | CXFA_FFExclGroup::~CXFA_FFExclGroup() = default;
      |                                         ^
../../v8/include/cppgc/garbage-collected.h:69:8: note: member found by ambiguous name lookup
   69 |   void operator delete[](void*) = delete;
      |        ^
../../v8/include/cppgc/garbage-collected.h:103:8: note: member found by ambiguous name lookup
  103 |   void operator delete[](void*) = delete;
      |        ^

I'm working on putting together a minimal repro.

Fznamznon · 2025-04-08T17:01:06Z

@DKLoehr , does clang that is being used also have #134357 in?

DKLoehr · 2025-04-08T17:28:48Z

Yes, I've reproduced it locally with current trunk clang.

DKLoehr · 2025-04-08T19:51:09Z

This is the smallest reproduction I could find:

struct BaseDelete1 {
  void operator delete[](void *);
};
struct BaseDelete2 {
  void operator delete[](void *);
};
struct BaseDestructor {
  virtual ~BaseDestructor() = default;
};
struct Final : BaseDelete1, BaseDelete2, BaseDestructor {};

When run with clang++ repro.cc -fsyntax-only -std=c++20, it yields the following error. Removing any one of the three base classes causes it to compile without problems.

repro.cc:10:8: error: member 'operator delete[]' found in multiple base classes of different types
   10 | struct Final : BaseDelete1, BaseDelete2, BaseDestructor {};
      |        ^
repro.cc:10:8: note: in implicit destructor for 'Final' first required here
repro.cc:2:8: note: member found by ambiguous name lookup
    2 |   void operator delete[](void *);
      |        ^
repro.cc:5:8: note: member found by ambiguous name lookup
    5 |   void operator delete[](void *);
      |        ^

Fznamznon · 2025-04-09T08:24:46Z

Looking... Does not reproduce without c++20.

And issue was reported in llvm#133950 . Since we don't always emit vector deleting dtors, only error out about ambigous operator delete[] when it will be required for vector delering dtor emission.

Fznamznon · 2025-04-09T16:08:31Z

I posted a PR with a potential fix.

#135041) And issue was reported in #133950 (comment) . Since we don't always emit vector deleting dtors, only error out about ambiguous operator delete[] when it will be required for vector deleting dtor emission.

…hen required (#135041) And issue was reported in llvm/llvm-project#133950 (comment) . Since we don't always emit vector deleting dtors, only error out about ambiguous operator delete[] when it will be required for vector deleting dtor emission.

DKLoehr · 2025-04-11T16:49:12Z

Thanks! Unfortunately it looks like the fix introduced a regression of #134265 on Windows. Repro:

struct Base {
  virtual ~Base();
  void operator delete[](void *) = delete;
};
class __declspec(dllexport) Derived : Base {};

Running clang-cl repro.cc /std:c++20 results in the following error after

repro.cc(5,29): error: attempt to use a deleted function
    5 | class __declspec(dllexport) Derived : Base {};
      |                             ^
repro.cc(5,29): note: in implicit destructor for 'Derived' first required here
repro.cc(5,18): note: due to 'Derived' being dllexported
    5 | class __declspec(dllexport) Derived : Base {};
      |                  ^
repro.cc(3,8): note: 'operator delete[]' has been explicitly marked deleted here
    3 |   void operator delete[](void *) = delete;
      |        ^

Fznamznon · 2025-04-11T17:08:15Z

Yeah, but this is kind of expected. Otherwise it is unclear how to emit vector deleting destructor body. Should we fallback to calling something, like MSVC does? See #135041 (comment) for example.

DKLoehr · 2025-04-11T19:16:38Z

I'm not familiar enough to say what we should be doing; perhaps @rnk or @zmodem have thoughts? It seems like we should at least emit a better error message, since just looking at the code doesn't make it clear to me why operator delete[] is relevant.

Since this is causing clang to become more restrictive, the best thing to do might be to revert to whatever we used to do in this situation, and then make a followup change which specifically explains why clang is becoming more restrictive.

zmodem · 2025-04-14T09:38:04Z

Thanks! Unfortunately it looks like the fix introduced a regression of #134265 on Windows. Repro:

(For reference, this is https://crbug.com/410000261 on our side.)

The "deleted operator delete[]" is coming from here: https://source.chromium.org/chromium/chromium/src/+/main:v8/include/cppgc/garbage-collected.h;l=61

Rejecting the code seems wrong, since that's only happening due to this extension. I'm not sure what the best option is here, but I think we should look closer at what exactly MSVC is emitting.

Fznamznon · 2025-04-14T09:41:10Z

I'm working on a revert of the extension.

…ator delete (llvm#133950)" This reverts commit 8a691cc.

Finding operator delete[] is still problematic, without it the extension is a security hazard, so reverting until the problem with operator delete[] is figured out. This reverts the following PRs: Reland [MS][clang] Add support for vector deleting destructors (#133451) [MS][clang] Make sure vector deleting dtor calls correct operator delete (#133950) [MS][clang] Fix crash on deletion of array of pointers (#134088) [clang] Do not diagnose unused deleted operator delete[] (#134357) [MS][clang] Error about ambiguous operator delete[] only when required (#135041)

MSVC supports and extension allowing to delete an array of objects via pointer whose static type doesn't match its dynamic type. This is done via generation of special destructors - vector deleting destructors. MSVC's virtual tables always contain pointer to vector deleting destructor for classes with virtual destructors, so not having this extension not having this extension implemented causes clang to generate code that is not compatible with the code generated by MSVC, because clang always puts pointer to a scalar deleting destructor to the vtable. As a bonus the deletion of an array of polymorphic object will work just like it does with MSVC - no memory leaks and correct destructors are called. This patch will cause clang to emit code that is compatible with code produced by MSVC but not compatible with code produced with clang of older versions, so the new behavior can be disabled via passing -fclang-abi-compat=21 (or lower). This is yet another attempt to land vector deleting destructors support originally implemented by llvm#133451. This PR contains fixes for issues reported in the original PR as well as fixes for issues related to operator delete[] search reported in several issues like llvm#133950 (comment) llvm#134265 Fixes llvm#19772

MSVC supports an extension allowing to delete an array of objects via pointer whose static type doesn't match its dynamic type. This is done via generation of special destructors - vector deleting destructors. MSVC's virtual tables always contain a pointer to the vector deleting destructor for classes with virtual destructors, so not having this extension implemented causes clang to generate code that is not compatible with the code generated by MSVC, because clang always puts a pointer to a scalar deleting destructor to the vtable. As a bonus the deletion of an array of polymorphic object will work just like it does with MSVC - no memory leaks and correct destructors are called. This patch will cause clang to emit code that is compatible with code produced by MSVC but not compatible with code produced with clang of older versions, so the new behavior can be disabled via passing -fclang-abi-compat=21 (or lower). This is yet another attempt to land vector deleting destructors support originally implemented by #133451. This PR contains fixes for issues reported in the original PR as well as fixes for issues related to operator delete[] search reported in several issues like #133950 (comment) #134265 Fixes #19772

…tors (#165598) MSVC supports an extension allowing to delete an array of objects via pointer whose static type doesn't match its dynamic type. This is done via generation of special destructors - vector deleting destructors. MSVC's virtual tables always contain a pointer to the vector deleting destructor for classes with virtual destructors, so not having this extension implemented causes clang to generate code that is not compatible with the code generated by MSVC, because clang always puts a pointer to a scalar deleting destructor to the vtable. As a bonus the deletion of an array of polymorphic object will work just like it does with MSVC - no memory leaks and correct destructors are called. This patch will cause clang to emit code that is compatible with code produced by MSVC but not compatible with code produced with clang of older versions, so the new behavior can be disabled via passing -fclang-abi-compat=21 (or lower). This is yet another attempt to land vector deleting destructors support originally implemented by llvm/llvm-project#133451. This PR contains fixes for issues reported in the original PR as well as fixes for issues related to operator delete[] search reported in several issues like llvm/llvm-project#133950 (comment) llvm/llvm-project#134265 Fixes llvm/llvm-project#19772

[MS][clang] Make sure vector deleting dtor calls correct operator delete

8b2aa22

During additional testing I spotted that vector deleting dtor calls operator delete, not operator delete[] when performing array deletion. This patch fixes that.

Fznamznon requested review from rnk, tahonermann and zmodem April 1, 2025 17:59

Make format happy

a05aaaf

rnk approved these changes Apr 1, 2025

View reviewed changes

Fznamznon merged commit 8a691cc into llvm:main Apr 2, 2025
11 checks passed

ilovepi mentioned this pull request Apr 3, 2025

Incorrect error reported for deleted function #134265

Closed

Fznamznon mentioned this pull request Apr 9, 2025

[MS][clang] Error about ambiguous operator delete[] only when required #135041

Merged

Fznamznon added a commit to Fznamznon/llvm-project that referenced this pull request Apr 14, 2025

Revert "[MS][clang] Make sure vector deleting dtor calls correct oper…

4f368fe

…ator delete (llvm#133950)" This reverts commit 8a691cc.

Fznamznon mentioned this pull request Oct 29, 2025

Reland [MS][clang] Add support for vector deleting destructors #165598

Merged

[MS][clang] Make sure vector deleting dtor calls correct operator delete #133950

[MS][clang] Make sure vector deleting dtor calls correct operator delete #133950

Uh oh!

Conversation

Fznamznon commented Apr 1, 2025

Uh oh!

github-actions bot commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rnk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DKLoehr commented Apr 8, 2025

Uh oh!

Fznamznon commented Apr 8, 2025

Uh oh!

DKLoehr commented Apr 8, 2025

Uh oh!

DKLoehr commented Apr 8, 2025

Uh oh!

Fznamznon commented Apr 9, 2025

Uh oh!

Fznamznon commented Apr 9, 2025

Uh oh!

DKLoehr commented Apr 11, 2025

Uh oh!

Fznamznon commented Apr 11, 2025

Uh oh!

DKLoehr commented Apr 11, 2025

Uh oh!

zmodem commented Apr 14, 2025

Uh oh!

Fznamznon commented Apr 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Apr 1, 2025 •

edited

Loading